Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add task solution #5836

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add task solution #5836

wants to merge 2 commits into from

Conversation

ihnatd
Copy link

@ihnatd ihnatd commented Dec 18, 2024

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your hard work on this project! 😊 However, there are a few critical issues that need to be addressed before we can approve the pull request. These include HTML syntax errors, such as missing spaces around attributes and misplaced tags, which could affect rendering. Additionally, the .hover class is not functioning as intended, and the CSS does not fully meet the task requirements. Please address these issues to ensure the code meets the core requirements. Keep up the great work, and don't hesitate to reach out if you need any help!

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

src/index.html Outdated
</a>
<nav>
<ul>
<li><a href="#"class ="is-active">Apple</a></li>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a space between href="#" and class="is-active". This is an HTML syntax issue that could affect rendering.

src/index.html Outdated
<li><a href="#"class ="is-active">Apple</a></li>
<li><a href="#">Samsung</a></li>
<li><a href="#">Smartphones</a></li>
<li><a href="#"class ="hover"></a>Laptops & Computers</a></li>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a misplaced closing tag here. The </a> tag should be placed after the text Laptops & Computers, not before it. Also, ensure there is a space between href="#" and class="hover".

Comment on lines +67 to +70
nav a.hover {
position: relative;
color: #007bff;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .hover class is defined, but it might not work as expected because it's not associated with any hover state or JavaScript interaction. Consider using the :hover pseudo-class if you intend to change the style on hover.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great effort on your submission! 🎉 While there are some areas that need improvement, such as the inconsistent spacing around attributes in your HTML and the non-functional .hover class in your CSS, you've made significant progress. Remember to use the :hover pseudo-class for hover effects and ensure your CSS is optimized by avoiding the universal * selector. Also, make sure to include all necessary files, like images/logo.png, and use CSS variables for colors as required. Keep up the good work and address these points to enhance your code quality. This approval is to encourage your learning journey, but please make sure to fix these issues in future projects to avoid similar problems. Keep improving and learning from these experiences! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

@@ -17,6 +17,22 @@
/>
</head>
<body>
<h1>Moyo header</h1>
<header>
<a href = "#" class="logo">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra space around the = in the href attribute. It should be href="#" for consistency and to avoid potential issues.

</a>
<nav>
<ul>
<li><a href="#" class ="is-active">Apple</a></li>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra space around the = in the class attribute. It should be class="is-active" for consistency.

<li><a href="#" class ="is-active">Apple</a></li>
<li><a href="#">Samsung</a></li>
<li><a href="#">Smartphones</a></li>
<li><a href="#" class ="hover">Laptops & Computers</a></li>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .hover class is not functioning as intended. Please check the CSS file to ensure that the class is defined correctly and that the styles are applied as expected.

Comment on lines +67 to +70
nav a.hover {
position: relative;
color: #007bff;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .hover class is defined here, but it seems to be intended for hover effects. Consider using the :hover pseudo-class instead to apply styles when the user hovers over the element. For example, nav a:hover { color: #007bff; }.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants